-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DISCO-3302] Migrate to uv for package and environment management #809
base: main
Are you sure you want to change the base?
Conversation
c44d04f
to
88a33b9
Compare
python-version: "3.12" | ||
cache: "poetry" | ||
python-version-file: "pyproject.toml" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately we can't use the official uv
github action (astral-sh/setup-uv@v5
) because it is not approved by SRE. Hence, using pipx
to manually install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you note this in the file perhaps with the SRE link so we can follow up later?
[build-system] | ||
requires = ["hatchling"] | ||
build-backend = "hatchling.build" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file was auto-generated / formatted by the uvx migrate-to-uv
command.
with ( | ||
patch("google.cloud.storage.Client") as mock_client, | ||
patch("google.auth.default") as mock_auth_default, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting
with ( | ||
patch("merino.utils.gcs.gcs_uploader.Client") as mock_client, | ||
patch("google.auth.default") as mock_auth_default, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting
with ( | ||
patch("google.cloud.storage.Client") as mock_client, | ||
patch("google.auth.default") as mock_auth_default, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting
|
||
from merino.utils.version import Version, fetch_app_version_from_file | ||
|
||
|
||
def test_fetch_app_version_from_file() -> None: | ||
"""Happy path test for fetch_app_version_from_file().""" | ||
expected_information: dict = { | ||
"source": "https://github.com/mozilla-services/merino-py", | ||
"source": HttpUrl("https://github.com/mozilla-services/merino-py"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't failing on main
but I think uv
installed a newer version of mypy
which is asserting on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this is also how the type for source
key i.e source: HttpUrl ...
defined in the source code.
python-version: "3.12" | ||
cache: "poetry" | ||
python-version-file: "pyproject.toml" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you note this in the file perhaps with the SRE link so we can follow up later?
RUN pip install --no-cache-dir --quiet poetry-plugin-export | ||
# Install build dependencies for MaxMindDB | ||
RUN apt-get update && \ | ||
apt-get install --yes build-essential libmaxminddb0 libmaxminddb-dev && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both libmaxminddb0
and libmaxminddb-dev
are C extensions for geoip2
hence are required for the stage app_base
.
# Copy local code to the container image. | ||
COPY . $APP_HOME | ||
# Copy the virtual environment from the build stage | ||
COPY --from=build --chown=app:app /opt/venv /opt/venv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need uv
here to activate the virtual environment for the container.
When I tried the container locally via:
$ docker build -t merino-py .
$ docker run --rm -d -p 8000:8000 -e MERINO_ENV=production --name merino-py merino-py
docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: exec: "uvicorn": executable file not found in $PATH: unknown
$ docker run --rm -it --entrypoint bash merino-py
# Run this inside the container
$ python
> import geoip2.database
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'geoip2'
|
||
COPY --from=build /tmp/requirements.txt $APP_HOME/requirements.txt | ||
# Copy the application code | ||
COPY . $APP_HOME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're here, could you add .ruff_cache
to .dockerignore
? Just noticed that it sneaked into my local docker image.
References
JIRA: DISCO-3302
Description
Migrate to
uv
as the package and environment manager. Goal is to move away from frompoetry
andpyenv
and consolidate everything by usinguv
only, which is also much faster.Note
Unblocks #799
PR Review Checklist
Put an
x
in the boxes that apply[DISCO-####]
, and has the same title (if applicable)[load test: (abort|skip|warn)]
keywords are applied to the last commit message (if applicable)